[SPARK-20164][SQL] AnalysisException not tolerant of null query plan. by kunalkhamar · Pull Request #17486 · apache/spark

kunalkhamar · 2017-03-30T21:13:41Z

What changes were proposed in this pull request?

The query plan in an AnalysisException may be null when an AnalysisException object is serialized and then deserialized, since plan is marked @transient. Or when someone throws an AnalysisException with a null query plan (which should not happen).
def getMessage is not tolerant of this and throws a NullPointerException, leading to loss of information about the original exception.
The fix is to add a null check in getMessage.

How was this patch tested?

Unit test

gatorsmile · 2017-03-30T22:08:12Z


  override def getMessage: String = {
-    val planAnnotation = plan.map(p => s";\n$p").getOrElse("")
+    val planAnnotation = if (plan == null) "" else plan.map(p => s";\n$p").getOrElse("")


Nit:

val planAnnotation = Option(plan).map(p => s";\n$p").getOrElse("")

gatorsmile · 2017-03-30T22:09:57Z

We should pass None for the parm plan, instead of null. I think we need to fix the place we throw the AnalysisException if any code passes null.

kunalkhamar · 2017-03-30T22:57:55Z

@gatorsmile Looks like its happening because plan is marked @transient, when AnalysisException is serialized and then deserialized, plan may turn up null.

SparkQA · 2017-03-30T23:32:19Z

Test build #75403 has finished for PR 17486 at commit e3c1970.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-03-31T00:10:29Z

Test build #75407 has finished for PR 17486 at commit 1606320.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-03-31T00:19:59Z

Test build #75404 has finished for PR 17486 at commit 14fd95d.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-03-31T02:56:13Z


  override def getMessage: String = {
-    val planAnnotation = plan.map(p => s";\n$p").getOrElse("")
+    val planAnnotation = Option(plan).map(p => s";\n$p").getOrElse("")


Sorry, my fault. We need another flatten.

val planAnnotation = Option(plan).flatten.map(p => s";\n$p").getOrElse("")

Right, updated.

SparkQA · 2017-03-31T05:33:22Z

Test build #75415 has finished for PR 17486 at commit c8aebe7.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-03-31T05:38:21Z

LGTM

It sounds like the support of the default value in Scala is weird. I did some searches. Scala omits the default value. Another alternative is to define auxiliary constructor function to explicitly set the default value.

cc @rxin @hvanhovell @cloud-fan

viirya · 2017-03-31T05:43:50Z

LGTM

A variable marked with @transient will be restored to the default value of the type, not the default value of the parameter.

Quoted from Chapter 25 of Programming in Scala, First Edition: http://www.artima.com/pins1ed/annotations.html

Finally, Scala provides a @transient annotation for fields that should not be serialized at all. If you mark a field as @transient, then the framework should not save the field even when the surrounding object is serialized. When the object is loaded, the field will be restored to the default value for the type of the field annotated as @transient.

gatorsmile · 2017-03-31T16:16:26Z

@kunalkhamar Could you summarize the issue and post it in https://github.com/databricks/scala-style-guide? Thanks!

The query plan in an `AnalysisException` may be `null` when an `AnalysisException` object is serialized and then deserialized, since `plan` is marked `transient`. Or when someone throws an `AnalysisException` with a null query plan (which should not happen). `def getMessage` is not tolerant of this and throws a `NullPointerException`, leading to loss of information about the original exception. The fix is to add a `null` check in `getMessage`. - Unit test Author: Kunal Khamar <kkhamar@outlook.com> Closes #17486 from kunalkhamar/spark-20164. (cherry picked from commit 254877c) Signed-off-by: Xiao Li <gatorsmile@gmail.com>

gatorsmile · 2017-03-31T16:21:22Z

Thanks! Merging to master and 2.1.

kunalkhamar · 2017-03-31T22:48:45Z

@gatorsmile Verified the behaviour using this, it makes plan null upon deserialization.

import java.io._
import org.apache.spark.sql.AnalysisException

lazy val exception = new AnalysisException("", None, None, plan = None)
// Serialize exception
lazy val bo = new ByteArrayOutputStream()
lazy val o = new ObjectOutputStream(bo)
o.writeObject(exception)
lazy val bytes = bo.toByteArray

// Deserialize ex
lazy val bi = new ByteArrayInputStream(bytes)
lazy val i = new ObjectInputStream(bi)
lazy val deserialized = i.readObject.asInstanceOf[AnalysisException]

println(deserialized.plan)

Not sure what to add and where in the scala-style-guide?

kunalkhamar added 2 commits March 30, 2017 14:11

Add null check to AnalysisException.

e3c1970

Add a unit test.

14fd95d

kunalkhamar changed the title ~~[SPARK-20164][SQL][WIP] AnalysisException not tolerant of null query plan.~~ [SPARK-20164][SQL] AnalysisException not tolerant of null query plan. Mar 30, 2017

gatorsmile reviewed Mar 30, 2017

View reviewed changes

Change from review.

1606320

gatorsmile reviewed Mar 31, 2017

View reviewed changes

Flatten nested option.

c8aebe7

asfgit closed this in 254877c Mar 31, 2017

Conversation

kunalkhamar commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

gatorsmile Mar 30, 2017

Choose a reason for hiding this comment

Uh oh!

kunalkhamar Mar 30, 2017

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kunalkhamar commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Mar 30, 2017

Uh oh!

SparkQA commented Mar 31, 2017

Uh oh!

SparkQA commented Mar 31, 2017

Uh oh!

gatorsmile Mar 31, 2017

Choose a reason for hiding this comment

Uh oh!

kunalkhamar Mar 31, 2017

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 31, 2017

Uh oh!

gatorsmile commented Mar 31, 2017

Uh oh!

viirya commented Mar 31, 2017

Uh oh!

gatorsmile commented Mar 31, 2017

Uh oh!

gatorsmile commented Mar 31, 2017

Uh oh!

kunalkhamar commented Mar 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kunalkhamar commented Mar 30, 2017 •

edited

Loading

gatorsmile commented Mar 30, 2017 •

edited

Loading

kunalkhamar commented Mar 30, 2017 •

edited

Loading